Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Get rid of the dotmap and generate Munches for multsigs and extras too. #72

Merged
merged 13 commits into from
Jul 27, 2023

Conversation

Tritium-VLK
Copy link
Member

No description provided.

@Tritium-VLK Tritium-VLK requested a review from SHAKOTN July 25, 2023 20:23
Copy link
Member

@SHAKOTN SHAKOTN left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see implementation of self.populate_extras() and self.populate_multisigs(). Accessing property will literally throw an exception

bal_addresses/addresses.py Show resolved Hide resolved
bal_addresses/addresses.py Show resolved Hide resolved
@Tritium-VLK Tritium-VLK requested a review from SHAKOTN July 26, 2023 21:46
@Tritium-VLK
Copy link
Member Author

ok @SHAKOTN please take another look.

Tritium added 9 commits July 27, 2023 00:33
CHANGELOG.md Outdated Show resolved Hide resolved
self._multisigs = Munch.fromDict(msigs[self.chain])
else:
print(f"Warning: No multisigs for chain {self.chain}, multisigs must be added in extras/multisig.json")
self._multisigs = {}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make it empty Munch object to unify the interface

bal_addresses/errors.py Outdated Show resolved Hide resolved
@@ -76,10 +105,36 @@ def test_deployments_invalid_format():
}
}
)
responses.add(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nicely done, but please pay attention to the name of the test.

It's always good to make tests as small as possible.

My suggestion: create separate test functions for both extras and msigs logic

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried that but you have to copy all the inputs each time and then there is very little additional code. It felt silly and made changes to the test inputs hard.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should separate all the test cases in smaller units, that is my recommendation. It's better to NOT test everything in one function even if it leads to boilerplate code

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another day ser :)

@Tritium-VLK Tritium-VLK merged commit 31f06df into main Jul 27, 2023
6 checks passed
@SHAKOTN SHAKOTN deleted the add_other_addresses branch July 27, 2023 11:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants